Skip to content

Potentially risky but... first generation laser can failure detection#338

Open
Rongrrz wants to merge 1 commit intomainfrom
Elevator-lasercan-fallback
Open

Potentially risky but... first generation laser can failure detection#338
Rongrrz wants to merge 1 commit intomainfrom
Elevator-lasercan-fallback

Conversation

@Rongrrz
Copy link
Copy Markdown
Contributor

@Rongrrz Rongrrz commented Mar 29, 2025

Why are we doing this?

Asana task URL:

Whats changing?

Questions/notes for reviewers

How this was tested

  • tested on robot
  • tested in simulator
  • unit tests added

Video/screenshots (from simulator or live robot)


PR feedback legend

Symbol Meaning
⭐ ⭐ ⭐ must be addressed
⭐ ⭐ should be addressed
something to consider, a good idea

@Rongrrz Rongrrz requested review from a team and junyu101 as code owners March 29, 2025 20:31
Copy link
Copy Markdown
Contributor

@JohnGilb JohnGilb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For this 1st generation, just use the akitLog to record this "I want to disable the LaserCAN" value to see if this triggers at all during normal operation.

My main concern is that noise could trigger this even when nothing surprising is happening.

masterMotor.setTrapezoidalProfileJerk(RadiansPerSecondPerSecond.of(motionMagicJerk.get()).per(Second));
}

public void setUseDistanceSensor(boolean value) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⭐ for callers, it's nice when the argument is self-documenting (e.g. instead of value, something like useLaserCANSensor)

// We are in a state of not good stuff and should disable the distance sensor
if (previousLaserCanDistance.minus(d).in(Meters) > laserCanOffsetThreshold.get()
&& masterMotor.getRawVelocity().baseUnitMagnitude() > 0) {
if (badMotorLaserCanHarmonyCount < harmonyThreshold) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: suggest renaming to badHarmonyBetweenLaserCanAndMotorCount ; it's longer but makes it clear that harmony refers to the relationship between these two devices.

@Rongrrz Rongrrz added the DONT MERGE This not safe to merge, don't do it! label Apr 2, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

DONT MERGE This not safe to merge, don't do it!

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants